Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: duplicated word in seedphrase #1337

Merged
merged 22 commits into from
Feb 19, 2020
Merged

Conversation

estebanmino
Copy link
Contributor

@estebanmino estebanmino commented Feb 11, 2020

Description

Fix for issue with duplicated words on seedphrase

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Fixes #1233
Fixes #1020

@estebanmino estebanmino added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Feb 11, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

built release mode to both OS's, all looks good, QA Passed 👍


findNextAvailableIndex = () => {
const { confirmedWords } = this.state;
for (let i = 0; i < 12; i++) if (!confirmedWords[i].word) return i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of

for (let i = 0; i < 12; i++) if (!confirmedWords[i].word) return i;
return 12;

could do

return confirmedWords.findIndex(obj => !word)

and then use this.findNextAvailableIndex() === -1 on line 218

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

createWordsDictionary = () => {
const dict = {};
this.words.forEach((word, i) => {
dict[[word, i]] = { currentPosition: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not exactly clear to anyone ready the code that the keys of dict will be strings of the form 'word,i'

The code could be a bit clearer if the keys were explicitly set with strings.

For example:

dict[`${word},${i}`] = { currentPosition: undefined };

It would then be clearer what is happening when the key is split on line 280

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it if it's not clear

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed code. It all looks good and that it works fine. Made a couple comments on code improvements that can be made, but they don't need to block merging.

@estebanmino
Copy link
Contributor Author

@ibrahimtaveras00 can you do other QA pass? I've changed the code, just to be sure. I'm pushing the hardcoded seed phrase for you to test it.

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latest updates look good, QA passed 👍

@estebanmino estebanmino merged commit 8fb1a6b into develop Feb 19, 2020
@whymarrh whymarrh deleted the bugfix/duplicated-word branch March 19, 2020 03:07
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* progress

* cleaner code

* fix check

* works

* almost there

* fix some bugs

* cleaner

* select empty box

* done

* undo custom changes

* Revert "undo custom changes"

This reverts commit 774113e.

* Revert "Revert "undo custom changes""

This reverts commit 468ae85.

* fix bug

* snaps

* Revert "undo custom changes"

This reverts commit 774113e.

* Revert "Revert "undo custom changes""

This reverts commit 54acd16.

* review comments

* seedPhraseReady

* Revert "Revert "undo custom changes""

This reverts commit 54acd16.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
3 participants